Structural Refactor #87
Conversation
|
After testing it through a private functional test, I would say the effort to make this refactor really pays off. It's been a game-changer for my development process. Before that, I had to include every suite possible, which was a real headache and made the codebase unnecessarily bloated. Now, I can just choose whatever crypto suite I want, say for my ESP32 code now. It's given me so much more flexibility and control over my projects. While I didn't get a chance to run a real test on the ESP32 hardware itself (which is definitely on my to-do list), I used the following setting in my Windows binary test to simulate the environment: This is a bare-minimum dual TLS1.3/TLS1.2 compliant setting, using only TLS13_CHACHA20_POLY1305_SHA256 and TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256 (for TLS1.2). But if I ran only TLS1.3, it would shave off about 300KB from the binary, down from 1MB to 713KB (30% binary size saved!). That's a scary amount. It's wild to think about how much space these security protocols take up. This approach allowed me to get a good feel for how the refactored code would perform in a more constrained environment like the ESP32. The results were promising, showing improved efficiency and reduced overhead. It's really exciting to see how this change could potentially optimize performance on embedded systems. I mean, 300KB might not seem like a lot in today's world of gigabyte-sized apps, but when you're working with embedded systems or trying to optimize for performance, every kilobyte counts. And let's be real, the fact that dropping down to only using TLS1.3 and just one suite saves that much space is kind of mind-blowing. It really makes you wonder about the trade-offs between security and efficiency, especially in resource-constrained environments. I guess that's the price we pay for keeping our data safe in transit, right? PS: The environment is simulated using Rustls unbuffered API, which is for running in an embedded, no_std but alloc available environment. Together with this provider being no_std friendly, this is probably the first publicly-maintained provider that could run on MCUs officially. |
|
@stevefan1999-personal it looks like there are conflicts with |
|
@tarcieri the problem is that I re-sorted the entries, so it is not in symphony deliberately...maybe this could be another PR |
|
@tarcieri Maybe merge it soon? btw a new challenger approaches: https://github.com/wolfSSL/rustls-wolfcrypt-provider (also give me the invite for the maintainer status again i didnt receive the invite as i was told in zulip) |
Ah, yes. That's because I used Linux at my workplace, and Windows at home, at the same time I have autocrlf turned on... |
|
Also given the chance, I think we should shamefully steal the code from https://github.com/cryspen/hpke-rs :) @cryspen @tarcieri not sure if you guys would be happy about it |
|
I've managed to get QUIC working. Hooray! |
|
Could we please maybe by any chance pretty please :) split this PR up perhaps given there are a lot of unrelated changes making it harder to review and keep track of ? There are review comments buried in the middle that are unresolved (see the hidden items) The PR doesn't compile currently btw ? |
| @@ -1,37 +1,41 @@ | |||
| name: validate-local-openssl | |||
There was a problem hiding this comment.
unrelated change and has <CR><LF> endings with github confusing it.
If we need to change github CI unrelated to this PR, I would recommend doing it in a separate PR
There was a problem hiding this comment.
See the build.rs in openssl validation with Makefile that generates the key where we don't put key/cert (esp with expiry date) artefacts themselves to git.
same as https://github.com/RustCrypto/rustls-rustcrypto/pull/87/files#r2361803656
and what we did in #66
| p521 = { version = "0.14.0-pre.11", default-features = false, optional = true } | ||
| pkcs1 = { version = "0.8.0-rc.4", default-features = false, optional = true } | ||
| pkcs8 = { version = "0.11.0-rc.7", default-features = false, optional = true } | ||
| rsa = { version = "0.10.0-rc.9", default-features = false, optional = true } |
There was a problem hiding this comment.
Getting this bumped up would be interesting re:
I wonder if the new rsa suffers from as much as 600ms hang-up upon negotiation (other key types 6-11 ms max)
Also not sure why rsa is optional here? it's MTI ?
| webpki = { package = "rustls-webpki", version = "0.103.6", default-features = false, optional = true } | ||
| enum_dispatch = "0.3.13" | ||
| tinyvec = { version = "1.10.0", default-features = false, optional = true } | ||
| thiserror = { version = "2.0.17", default-features = false } |
There was a problem hiding this comment.
thiserror not sure it's included in any other rustcrypto projects. seems like unnecessary dependency that contains unsafe.
There was a problem hiding this comment.
hmm, would you like to see if https://docs.rs/derive_more/latest/derive_more/ could be a good replacement? I won't mind as I actually use both
| rustls = { version = "0.23.32", default-features = false } | ||
| webpki = { package = "rustls-webpki", version = "0.103.6", default-features = false, optional = true } | ||
| enum_dispatch = "0.3.13" | ||
| tinyvec = { version = "1.10.0", default-features = false, optional = true } |
There was a problem hiding this comment.
why are tinyvec / enum_dispatch required?
There was a problem hiding this comment.
Because of this
#[enum_dispatch(MaskSample)]
#[allow(clippy::large_enum_variant)]
pub enum HeaderProtectionKey {
#[cfg(feature = "aes")]
Aes128Ecb(aes::Aes128),
#[cfg(feature = "aes")]
Aes256Ecb(aes::Aes256),
#[cfg(feature = "chacha20")]
ChaCha20(chacha20::Key),
}
#[enum_dispatch]
trait MaskSample {
fn new_mask(&self, sample: &[u8]) -> Result<[u8; 5], QuicError>;
}
That's a tricky hack to do QUIC mask generation without using dynamic dispatch aka Box<dyn MaskSample>, nor having to use a lot of verbosive generic black magic :) And it works surprisingly well, although at the cost of getting the size of HeaderProtectionKey the largest when AES-256 is enabled, but the enum size is up to that since behind the scene it is a tagged union. In exchange this can be pinned to a stack, making it wholly efficient and neat.
I actually did this before (before knowing enum_dispatch, so I hand rolled the enum match) and then I realized it is just doing what enum_dispatch is doing. I can always remove the need for enum_dispatch, but I don't like duplicating code.
| # External groups | ||
| pki-types = { package = "rustls-pki-types", version = "1.12.0", default-features = false } | ||
| rand_core = { version = "0.9.3", default-features = false, features = [ | ||
| "os_rng", |
There was a problem hiding this comment.
os_rng should be optional over just getrandom ? not sure what os_rng did beyond dep:getrandom as it's defined in rand_core ? in no-std environments custom has to be provided through getrandom as it was previously. ref: #86 (comment) earlier talk about getrandom.
There was a problem hiding this comment.
That's because you would still have something like EphemeralSecret::<C>::try_from_rng(&mut OsRng), that means OsRng would be here anyway, and I don't want to add too much crates with duplicated features to KISS
| pkcs1 = { version = "0.8.0-rc.4", default-features = false, optional = true } | ||
| pkcs8 = { version = "0.11.0-rc.7", default-features = false, optional = true } | ||
| rsa = { version = "0.10.0-rc.9", default-features = false, optional = true } | ||
| sec1 = { version = "0.8.0-rc.10", default-features = false, optional = true } |
There was a problem hiding this comment.
I thought we were not going to support SEC1 ? and and just go with PKCS#8
There was a problem hiding this comment.
I remember TLS1.3 did discourage the use of SEC1 but did TLS1.2 still support it? If so it is better to keep it. I have this as optional feature anyway until you enabled it, so maybe I need to say like add another feature flag to indicate the inclusion of SEC1 by hand explicitly too (in other words opt-in)
| kx-p384 = ["kx", "p384", "kx-nist", "p384/ecdh"] | ||
| kx-p521 = ["kx", "p521", "kx-nist", "p521/ecdh"] | ||
| kx-full = ["kx-x448", "kx-x25519", "kx-p256", "kx-p384", "kx-p521"] | ||
|
|
There was a problem hiding this comment.
These features change the behavior of the whole library without bifurcation across the whole build.
In a remote case if there are two things that pull rustcrypto-rustls they are able to add more features transparently to the other thing that pulls only set of limited features without it knowing the other pulled more features affecting it as well.
Also relaying these features through transient dependencies would be a no-go for libraries and typically this type of stuff would be configured by the top level binary only through use of cfg's.
I'm not sure it's best idea to a) have large feature list b) making things optional this way that should be mandatory and c) where we should enforce mandatory minimums.
It also makes maintenance, reviewability and testing much harder given one needs to test all the possible testable combinations where the feature knob list is tens of items long with each of them having 4-5 or so sub-items.
There was a problem hiding this comment.
I took this inspiration straight out from mbedTLS that they allow us to compose the TLS suites you wanted by enabling/disabling algorithms, so I would want to keep it, and it is also quite useful because in embedded context, you want to have this level of fine-grained control
| "elliptic-curve?/alloc", | ||
| "pkcs8?/alloc", | ||
| "sec1?/alloc", | ||
| "signature?/alloc", |
There was a problem hiding this comment.
Are (all) these optional alloc features required from (all) upstream deps and / or do they provide both any required functionality to downstream and to which we latch to with our usage given each crate ? Given a feature knob should not change behavior (genearlly) but provide extended API surface (exception being zeroize implementing Drop which is also additional code though kinda)
| # Cryptographic dependencies | ||
| aead = { version = "0.6.0-rc.2", default-features = false, optional = true } | ||
| aes = { version = "0.9.0-rc.1", default-features = false, optional = true } | ||
| aes-gcm = { version = "0.11.0-rc.1", default-features = false, optional = true } |
There was a problem hiding this comment.
why are aes/aes-gcm optional ? they are involved with MTI's ?
Sure, but I couldn't do it this week as I have a huge project up ahead.
Let me see them later
Oh that's because of the ESP-IDF Rust SDK and not related to the provider itself, let me see how to fix it |
|
I would definitely be a fan of splitting this up into smaller, more focused PRs. It's hard to review in the current form, and there is already so much discussion it's easy to get lost |
4 participants
This PR represents a monumental refactoring effort that transforms
rustls-rustcryptofrom its initial alpha implementation into an almost production-ready, highly modular TLS provider, if performance is not of concern. Spanning 70+ commits over an extended development cycle, this overhaul addresses fundamental architectural issues and delivers a pure-Rust TLS solution optimized for diverse deployment scenarios.📈 Evolution Timeline
Phase 1: Foundation (Initial Implementation)
Phase 2: Feature Expansion & Stability
no_stdcompatibility improvementsPhase 3: Major Structural Refactor
Phase 4: Production Readiness
🔧 Key Technical Changes
🏗️ Architectural Improvements
🔐 Cryptographic Enhancements
🛠️ Infrastructure & Quality
🧪 Testing & Validation
no_stdenvironments)🎯 Benefits & Impact
⚡ Performance & Efficiency
🔧 Developer Experience
🏭 Production Readiness
📋 Migration Guide
Breaking Changes:
Migration Steps:
Cargo.toml✅ Validation Results
This refactoring marks the culmination of extensive development effort, delivering a TLS provider that rivals commercial offerings while maintaining the simplicity and security of pure Rust implementation. The modular architecture ensures long-term maintainability while the comprehensive feature set supports everything from embedded IoT devices to high-performance web servers.